Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[script.plexmod] 0.7.8 #2620

Closed
wants to merge 1 commit into from
Closed

[script.plexmod] 0.7.8 #2620

wants to merge 1 commit into from

Conversation

pannal
Copy link

@pannal pannal commented May 20, 2024

Description

0.7.8

Checklist:

  • My code follows the add-on rules and piracy stance of this project.
  • I have read the CONTRIBUTING document
  • Each add-on submission should be a single commit with using the following style: [script.foo.bar] 1.0.0

@pannal
Copy link
Author

pannal commented May 20, 2024

Addon checker fails weirldly.

@basrieter
Copy link
Contributor

The checker is working again. Still an error, can you check that and fix it?

@pannal
Copy link
Author

pannal commented Jun 16, 2024

Heyho, not to push anything, but how's the cadence of merging PRs in this repo? Is it random or organized? Thanks!

@Hitcher
Copy link

Hitcher commented Jun 16, 2024

Heyho, not to push anything, but how's the cadence of merging PRs in this repo?

Whenever someone has the free time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a definite no go. We do not allow addons to write into Kodi config files or databases directly. All intractions with Kodi core must be done via official APIs.

Copy link
Author

@pannal pannal Jul 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only use this method to write the advancedsettings.xml when there's no official API available, such as for cache settings in Kodi older than 21, and <hosts> entries. The user has full control over this and we only touch the relevant areas and don't modify other unrelated entries in that xml file.

For anything above Kodi 20, this manages the <hosts> entries for user convenience to resolve plex.direct domains to avoid DNS rebind issues for novice users who don't know how to whitelist plex.direct in their routers.

Also I can't find any guideline stating that we can't write certain files. If that's in fact the case, why does xbmcvfs allow the write operation?

If you're referring to:

add-ons should store all their data in their own subfolder inside the addon_data directory. Access (read/write/delete) to any other files / folders is not allowed by default.
Exceptions to this rule may be granted in specific cases only. Please contact Team XBMC's add-on repository maintainers on github if your add-on needs access to such files.
In case we grant such an exception for your addon, access (read/write/delete) to other files/directories must be opt-in by the user, and be clear for the user to understand what is being accessed.

from https://kodi.wiki/view/Add-on_rules#Requirements_for_all_python_addons, I hereby request this addon to be whitelisted for this special case, please.

FYI: This functionality hasn't been added in that version, it was already in the accepted 0.7.6, just buried further in the code.

Edit: Corrections

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was added in a previous version this was obviously an ovelook from our (probably mine) side because of the size of the original PR. Regarding granting an exempt for your addon, I'll consult with other team members.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Author

@pannal pannal Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify: I can drop the cache management support for Kodi below 21, that's fine, if that's your main concern, as we're changing a very global setting.

We need the <hosts> management part of the advancedsettings.xml, though, as the plex.direct hosts change with every IP change and it's non-trivial for the user to find those hosts using the plex.tv API.

Telling the user to whitelist plex.direct in their router's DNS rebind protection configuration doesn't really solve the issue, either, because many provider-issued routers don't allow that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any news on this? Can I do anything to make this mergable? Again, I can drop the cache management stuff.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applogize for this taking way too long. Unfortunately, currently I have little time because of various life stuff.

So let's agree on dropping the cache management stuff. As I understand, hosts management is Plex specific and should not affect other addons. On my side I will do my best fo finish my review by this weekend.

Copy link
Author

@pannal pannal Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your understanding and the offer. Would it help me dropping the 0.7.8 PR and creating a new one for 0.7.9 in about 2-3 weeks? Maybe that takes some pressure away from you, as well.

There have been so many changes and fixes in the meantime.

If not, I'll gladly branch away and make the necessary adjustments for 0.7.8.

Edit: Corrections

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's better to make a new PR with all the fresh stuff.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romanvm this would be the change to disallow advancedsettings.xml writing for caching when installed from the official Kodi repository: pannal/plex-for-kodi@178038e

Please confirm that this suites what we've agreed upon, thank you!

@pannal
Copy link
Author

pannal commented Jul 26, 2024

Closing for new PR in a couple of weeks

@pannal pannal closed this Jul 26, 2024
@pannal pannal deleted the matrix branch September 14, 2024 00:41
@pannal pannal mentioned this pull request Sep 14, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants